Skip to content

fix Arbitrary file write extracting an archive containing symbolic links on artifacts() lead to Path Traversal#430

Closed
odaysec wants to merge 1 commit intooasisprotocol:masterfrom
odaysec:patch-1
Closed

fix Arbitrary file write extracting an archive containing symbolic links on artifacts() lead to Path Traversal#430
odaysec wants to merge 1 commit intooasisprotocol:masterfrom
odaysec:patch-1

Conversation

@odaysec
Copy link
Copy Markdown

@odaysec odaysec commented Apr 22, 2025

if err = os.Symlink(header.Linkname, path); err != nil {

To fix the issue need to ensure that symbolic links are resolved and validated before creating them. This involves:

  1. Using filepath.EvalSymlinks to resolve any symbolic links in the header.Linkname path.
  2. Validating that the resolved path is within the intended extraction directory.
  3. Updating the code in the tar.TypeSymlink case to incorporate these checks.

The fix will ensure that symbolic links cannot be used to write files outside the intended directory, mitigating the vulnerability.

Extracting symbolic links from a malicious zip archive, without validating that the destination file path is within the destination directory, can cause files outside the destination directory to be overwritten. This can happen if there are previously-extracted symbolic links or directory traversal elements and links (..) in archive paths. This problem is related to the ZipSlip vulnerability which is detected by the go/zipslip query; please see that query's help for more general information about malicious archive file vulnerabilities. This query considers the specific case where symbolic links are extracted from an archive, in which case the extraction code must be aware of existing symbolic links when checking whether it is about to extract a link pointing to a location outside the target extraction directory.

POC

links are extracted from an archive using the syntactic filepath.Rel function to check whether the link and its target fall within the destination directory. However, the extraction code doesn't resolve previously-extracted links, so a pair of links like subdir/parent -> .. followed by escape -> subdir/parent/.. -> subdir/../.. leaves a link pointing to the parent of the archive root. The syntactic Rel is ineffective because it equates subdir/parent/.. with subdir/, but this is not the case when subdir/parent is a symbolic link.

package main

import (
	"archive/tar"
	"io"
	"os"
	"path/filepath"
	"strings"
        "oasisprotocol/cli"
)

func isRel(candidate, target string) bool {
	// BAD: purely syntactic means are used to check
	// that `candidate` does not escape from `target`
	if filepath.IsAbs(candidate) {
		return false
	}
	relpath, err := filepath.Rel(target, filepath.Join(target, candidate))
	return err == nil && !strings.HasPrefix(filepath.Clean(relpath), "..")
}

func unzipSymlink(f io.Reader, target string) {
	r := tar.NewReader(f)
	for {
		header, err := r.Next()
		if err != nil {
			break
		}
		if isRel(header.Linkname, target) && isRel(header.Name, target) {
			os.Symlink(header.Linkname, header.Name)
		}
	}
}

To fix this vulnerability, resolve pre-existing symbolic links before checking that the link's target is acceptable:

package main

func isRel(candidate, target string) bool {
	// GOOD: resolves all symbolic links before checking
	// that `candidate` does not escape from `target`
	if filepath.IsAbs(candidate) {
		return false
	}
	realpath, err := filepath.EvalSymlinks(filepath.Join(target, candidate))
	if err != nil {
		return false
	}
	relpath, err := filepath.Rel(target, realpath)
	return err == nil && !strings.HasPrefix(filepath.Clean(relpath), "..")
}

References

Zip Slip Vulnerability
Path Traversal
CWE-22

@netlify
Copy link
Copy Markdown

netlify Bot commented Apr 22, 2025

Deploy Preview for oasisprotocol-cli canceled.

Name Link
🔨 Latest commit 3756b66
🔍 Latest deploy log https://app.netlify.com/sites/oasisprotocol-cli/deploys/6806dc4502e9180008ebc90c

@kostko
Copy link
Copy Markdown
Member

kostko commented Aug 13, 2025

Thanks for bringing this up! An improved version of the PR which works for our use case where root filesystem templates are the things being extracted (and we cannot just forbid such symlinks) is in #572.

@kostko kostko closed this Aug 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants